-
-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SF-3162 Warn user when training and drafting sources are different #2982
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2982 +/- ##
=======================================
Coverage 82.04% 82.04%
=======================================
Files 544 544
Lines 31697 31712 +15
Branches 5155 5132 -23
=======================================
+ Hits 26005 26018 +13
- Misses 4925 4938 +13
+ Partials 767 756 -11 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! Overall it looks good. I like the languageCodesEquivalent
solution, it's simple and should cover the majority if not all cases we would see.
Rather than showing an additional warning message, what if we only show the "All languages are correct" notice if we determine there is a mismatch between training and drafting source languages? We could change the required languagesVerified
message to "I understand, proceed drafting with selected languages".
Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/confirm-sources/confirm-sources.component.ts
line 88 at r1 (raw file):
private sourceLanguagesAreCompatible(source: TranslateSource, other: TranslateSource): boolean { if (source.writingSystem.tag === other.writingSystem.tag) return true; return this.i18nService.languageCodesEquivalent(source.writingSystem.tag, other.writingSystem.tag);
nit I wonder if this should take a source[]
that we loop through and compare source[0]
to each item? Doing this now should prevent us from having to update if/when we go to more than 1 alternate source.
Code quote:
private sourceLanguagesAreCompatible(source: TranslateSource, other: TranslateSource): boolean {
if (source.writingSystem.tag === other.writingSystem.tag) return true;
return this.i18nService.languageCodesEquivalent(source.writingSystem.tag, other.writingSystem.tag);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the "language confirmation" notice, the existing direction is to have the notice showing all the time with the hopes that the user checks and confirms the languages every time, as a safeguard. @Nateowami may have more to say on this.
Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/confirm-sources/confirm-sources.component.html
line 55 at r1 (raw file):
@if (showSourceLanguagesNotCompatibleError) { <app-notice type="error"> <span>{{ t("sources_must_be_same_language") }}</span>
If there are multiple training sources and their languages differ, we show a message on the main drafting page and prevent them from even getting to the first step. I believe we should follow this pattern, unless there's a good reason not to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR changes that requirement. We've confirmed with the Serval/EITL teams that if we verify the language codes are equivalent then draft quality will not be degraded.
I guess the question on keeping it as a safeguard for all instances would be if we believe selected sources have a high probability of having an incorrect language code set that needs reviewed before proceeding? Otherwise, I'd suggest that only displaying it when we detect a different language code alerts the user draft quality would be reduced but still allow them the option to proceed.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @josephmyers and @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/confirm-sources/confirm-sources.component.html
line 55 at r1 (raw file):
Previously, josephmyers wrote…
If there are multiple training sources and their languages differ, we show a message on the main drafting page and prevent them from even getting to the first step. I believe we should follow this pattern, unless there's a good reason not to.
This is a good point. I would consider multiple training sources with different language codes a hard stop as I don't believe Serval can handle it. I could go either way on the changes in the PR being a hard or soft stop. Serval will handle training and drafting sources being different language codes (but draft quality will be poor).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, cool. It sounds like you have more recent information than I do. Carry on!
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kylebuss and @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/confirm-sources/confirm-sources.component.html
line 55 at r1 (raw file):
Previously, kylebuss (Kyle Buss) wrote…
This is a good point. I would consider multiple training sources with different language codes a hard stop as I don't believe Serval can handle it. I could go either way on the changes in the PR being a hard or soft stop. Serval will handle training and drafting sources being different language codes (but draft quality will be poor).
Right. As the PR is currently, it is a hard stop, but it's hard stopping too late in the process. If we're going to hard stop, we need to hard stop at the "landing" page, where the other hard stops are.
Given that the risk/cost of a poor-quality draft is not significant and can be resolved by the user, we should default toward warning but not prohibiting users from choosing settings that look like they are incorrect. To clarify, the trade-off I'm assuming is either
The first bad outcome seems preferrable. |
Thanks for the considerations. I think the best way forward is to warn users on the confirm page when language codes are not equivalent between training and drafting languages, and then that means we do not have warnings on the generation page when their language codes do not match. The user will be able to proceed to generate the draft even if these warnings are visible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this makes sense. Can you update the JIRA task (title, description, acceptance tests, etc.), as well as updating this PR, to match this new direction?
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kylebuss and @RaymondLuong3)
8f68f24
to
17d5624
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am getting an error when starting a new draft.
Reviewable status: 3 of 12 files reviewed, 2 unresolved discussions (waiting on @RaymondLuong3)
Changing to be a draft |
This PR adds a warning for users when their training and drafting sources have different languages. It is not so simple just to compare language codes because some languages may have multiple valid codes. My approach was to take the Intl.DisplayNames() function and compare the English names if the codes are different. I am open to suggestions for better approaches.
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)